Gh 5121 fedx bind left join #5122

aschwarte10 · 2024-09-06T12:17:34Z

Note: The PR is still WIP (some further changes, and especially unit test coverage will follow) however it is ready for early review. The goal is to have this available in RDF4J 5.1

GitHub issue resolved: #5121

Briefly describe the changes proposed in this PR:

The PR consists of multiple commits which add an implementation for bind left joins. It is recommended to follow the change commit by commit (as the first ones are preparational refactoring)

Commit 1: GH-5121: refactor the bind join logic into a reusable base class

Refactor the existing logic for executing bind joins into a reusable
base class.

This change mostly moves the implementation logic from the existing
ControlledWorkerBindJoin class to a new intermediate implementation
(with the goal to make it reusable in a second step for left joins).

Note that the bind join implementation no longer uses the
ControlledWorkerJoin as base class, i.e. the decision of which join
implementation to use is moved to the strategy.

Commit 2: GH-5121: prepare execution of left joins in the federation strategy

Prepare to execute a specific implementation of a left join
implementation through the federation strategy.

Commit 3: GH-5121: implementation of left bind join operator

This change provides the implementation and activation for the left bind
join operator.

The algorithm is as follows:

execute left bind join using regular bound join query
process result iteration similar to BoundJoinVALUESConversionIteration
remember seen set of bindings (using index) and add original bindings
to those, i.e. put to result return all non-seen bindings directly from
the input

Note that the terminology in literature has changed to "bind joins".
Hence, for new classes and methods I try to follow that.

Change is covered with a unit tests

TODOs

further unit test coverage
validate check left bind join pattern
configurability to turn bind left joins off
...

PR Author Checklist (see the contributor guidelines for more details):

my pull request is self-contained
I've added tests for the changes I made
I've applied code formatting (you can use mvn process-resources to format from the command line)
I've squashed my commits where necessary
every commit message starts with the issue number (GH-xxxx) followed by a meaningful description of the change

Refactor the existing logic for executing bind joins into a reusable base class. This change mostly moves the implementation logic from the existing ControlledWorkerBindJoin class to a new intermediate implementation (with the goal to make it reusable in a second step for left joins). Note that the bind join implementation no longer uses the ControlledWorkerJoin as base class, i.e. the decision of which join implementation to use is moved to the strategy.

Prepare to execute a specific implementation of a left join implementation through the federation strategy.

This change provides the implementation and activation for the left bind join operator. The algorithm is as follows: - execute left bind join using regular bound join query - process result iteration similar to BoundJoinVALUESConversionIteration - remember seen set of bindings (using index) and add original bindings to those, i.e. put to result return all non-seen bindings directly from the input Note that the terminology in literature has changed to "bind joins". Hence, for new classes and methods I try to follow that. Change is covered with a unit tests

aschwarte10 · 2024-09-06T13:18:27Z

.../src/main/java/org/eclipse/rdf4j/federated/evaluation/join/ControlledWorkerBindJoinBase.java

+		return configuredBindJoinSize;
+	}
+
+	protected interface TaskCreator {


hmm, moving this interface up to the base class seems to cause a class incompatbility:

Sep 06, 2024 12:24:10 PM japicmp.output.incompatible.IncompatibleErrorOutput warn WARNING: Incompatibility detected: Requires semantic version level MAJOR: JApiMethod [oldMethod=org.eclipse.rdf4j.federated.evaluation.join.ControlledWorkerBoundJoin.determineTaskCreator(org.eclipse.rdf4j.query.algebra.TupleExpr,org.eclipse.rdf4j.query.BindingSet), newMethod=org.eclipse.rdf4j.federated.evaluation.join.ControlledWorkerBoundJoin.determineTaskCreator(org.eclipse.rdf4j.query.algebra.TupleExpr,org.eclipse.rdf4j.query.BindingSet), returnType=JApiReturnType [oldReturnTypeOptional=org.eclipse.rdf4j.federated.evaluation.join.ControlledWorkerBoundJoin$TaskCreator, newReturnTypeOptional=org.eclipse.rdf4j.federated.evaluation.join.ControlledWorkerBindJoinBase$TaskCreator, changeStatus=MODIFIED], getCompatibilityChanges()=[METHOD_RETURN_TYPE_CHANGED]] Sep 06, 2024 12:24:10 PM japicmp.output.incompatible.IncompatibleErrorOutput warn WARNING: Incompatibility detected: Requires semantic version level MAJOR: JApiMethod [oldMethod=org.eclipse.rdf4j.federated.evaluation.join.ControlledWorkerBoundJoin.setSubmitFirstResultImmediately(boolean), newMethod=n.a., returnType=JApiReturnType [oldReturnTypeOptional=void, newReturnTypeOptional=value absent, changeStatus=REMOVED], getCompatibilityChanges()=[METHOD_REMOVED]] Sep 06, 2024 12:24:10 PM japicmp.output.incompatible.IncompatibleErrorOutput warn WARNING: Incompatibility detected: Requires semantic version level MAJOR: JApiSuperclass [jApiClass=JApiClass [fullyQualifiedName=org.eclipse.rdf4j.federated.evaluation.join.ControlledWorkerBoundJoin, changeStatus=MODIFIED, compatibilityChanges=[METHOD_DEFAULT_ADDED_IN_IMPLEMENTED_INTERFACE]], oldSuperclass=org.eclipse.rdf4j.federated.evaluation.join.ControlledWorkerJoin, newSuperclass=org.eclipse.rdf4j.federated.evaluation.join.ControlledWorkerBindJoinBase, changeStatus=MODIFIED, compatibilityChanges=[SUPERCLASS_REMOVED]]

I will try to find a solution that remeains binary compatible

you might create the interface of the same name in the parent class and extend this one from that? This should preserve compatibility.

hmottestad · 2024-09-18T09:07:11Z

...ion/src/main/java/org/eclipse/rdf4j/federated/evaluation/iterator/BindLeftJoinIteration.java

+/**
+ * A {@link LookAheadIteration} for processing bind left join results.
+ *
+ * Algorithm:
+ *
+ * <ul>
+ * <li>execute left bind join using regular bound join query</li>
+ * <li>process result iteration similar to {@link BoundJoinVALUESConversionIteration}</li>
+ * <li>remember seen set of bindings (using index) and add original bindings to those, i.e. put to result return all
+ * non-seen bindings directly from the input</li>
+ *
+ *
+ * @author Andreas Schwarte
+ */


Can you add that this is used for OPTIONAL clauses. Is it used for anything else?

hmottestad · 2024-09-18T09:09:10Z

...ion/src/main/java/org/eclipse/rdf4j/federated/evaluation/iterator/BindLeftJoinIteration.java

+	protected BindingSet convert(BindingSet bIn, int bIndex) throws QueryEvaluationException {
+		QueryBindingSet res = new QueryBindingSet();
+		Iterator<Binding> bIter = bIn.iterator();
+		while (bIter.hasNext()) {


Could this be a for-each look?

for (Binding bs : bIn){ ... }

hmottestad · 2024-09-18T09:11:48Z

...ion/src/main/java/org/eclipse/rdf4j/federated/evaluation/iterator/BindLeftJoinIteration.java

+			int bIndex = Integer.parseInt(
+					bIn.getBinding(BoundJoinVALUESConversionIteration.INDEX_BINDING_NAME).getValue().stringValue());


Is the binding Value object ever a literal? So it would be possible to check if it's instanceof IntegerLiteral or something like that?

hmottestad · 2024-09-18T09:16:29Z

.../src/main/java/org/eclipse/rdf4j/federated/evaluation/join/ControlledWorkerBindJoinBase.java

+
+	protected final ControlledWorkerScheduler<BindingSet> scheduler;
+
+	protected final Phaser phaser = new Phaser(1);


I've seen in other code that we call phaser.forceTermination() in the handleClose method. Would that be wise here too?

hmottestad · 2024-09-18T09:26:38Z

...ion/src/main/java/org/eclipse/rdf4j/federated/evaluation/iterator/BindLeftJoinIteration.java

+	protected final CloseableIteration<BindingSet> iter;
+	protected final List<BindingSet> bindings;
+
+	protected Set<Integer> seenBindingIndexes = new HashSet<>();


Would the IntHashSet be appropriate here?

aschwarte10 · 2024-09-20T09:38:40Z

@hmottestad thanks for the feedback. I will get back to this PR soon (we are currently in the release finalization phase of our product and this requires most of my attention).

Quick question: is there a timeline for the 5.1 release? I would like to see this change included (if I get it ready) such that we can make use of it in our next release. Ideally the 5.1 release would be available in the second half of november. What are the current project plans?

hmottestad · 2024-09-20T10:03:58Z

We don't have any plans at the moment.

I'm currently working on the last SHACL paths that aren't supported. I also have a PR for configuring the Apache HttpClient timeouts. And there is a request for adding support for configuring the document loader in the new JSON-LD 1.1 parser.

Would really want the timeouts included, and if I start on the JSON-LD document loader then it shouldn't take more than a few weeks to get it merged.

So we can say mid november for RDF4J 5.1.

aschwarte10 added 3 commits September 6, 2024 14:12

GH-5121: prepare execution of left joins in the federation strategy

cc0fbcf

Prepare to execute a specific implementation of a left join implementation through the federation strategy.

aschwarte10 added the 📦 fedx fedx: optimized federated query support label Sep 6, 2024

aschwarte10 requested review from hmottestad and JervenBolleman September 6, 2024 12:17

aschwarte10 commented Sep 6, 2024

View reviewed changes

hmottestad reviewed Sep 18, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Gh 5121 fedx bind left join #5122

Gh 5121 fedx bind left join #5122

aschwarte10 commented Sep 6, 2024 •

edited

Loading

aschwarte10 Sep 6, 2024

jetztgradnet Sep 9, 2024

hmottestad Sep 18, 2024

hmottestad Sep 18, 2024

hmottestad Sep 18, 2024

hmottestad Sep 18, 2024 •

edited

Loading

hmottestad Sep 18, 2024

aschwarte10 commented Sep 20, 2024

hmottestad commented Sep 20, 2024

		int bIndex = Integer.parseInt(
		bIn.getBinding(BoundJoinVALUESConversionIteration.INDEX_BINDING_NAME).getValue().stringValue());


		protected final ControlledWorkerScheduler<BindingSet> scheduler;

		protected final Phaser phaser = new Phaser(1);

Gh 5121 fedx bind left join #5122

Are you sure you want to change the base?

Gh 5121 fedx bind left join #5122

Conversation

aschwarte10 commented Sep 6, 2024 • edited Loading

aschwarte10 Sep 6, 2024

Choose a reason for hiding this comment

jetztgradnet Sep 9, 2024

Choose a reason for hiding this comment

hmottestad Sep 18, 2024

Choose a reason for hiding this comment

hmottestad Sep 18, 2024

Choose a reason for hiding this comment

hmottestad Sep 18, 2024

Choose a reason for hiding this comment

hmottestad Sep 18, 2024 • edited Loading

Choose a reason for hiding this comment

hmottestad Sep 18, 2024

Choose a reason for hiding this comment

aschwarte10 commented Sep 20, 2024

hmottestad commented Sep 20, 2024

aschwarte10 commented Sep 6, 2024 •

edited

Loading

hmottestad Sep 18, 2024 •

edited

Loading